Support transactions #881

YordanPavlov · 2021-04-01T19:50:40Z

A pull request to expose the transactions support of LibrdKafka.

Since I noticed that support for transactions has been requested in this issue:
#871
I have done some work to expose it, please consider this pull request @iradul

Replication factor of transaction related options should be reduced to 1 so that we can test with a single broker.

YordanPavlov · 2021-04-01T19:55:54Z

I have struggled to find the best way to test the library specifying the git url in package.json. Seem in doing so the submodules are not present, do you have some guidelines on how to approach this.

edenhill · 2021-04-06T19:35:12Z

src/producer.cc

+    return Baton(RdKafka::ERR_NO_ERROR);
+  }
+  else {
+    Baton result(error->code());


The librdkafka transactional API uses the new richer rd_kafka_error_t type that provides some extra error attributes.
These attributes, fatal, txn_requires_abort and retriable, are necessary to expose in this node.js API as well since it tells the application how the errors are to be handled, which can not be derrived from the error code itself.

Thank you for your feedback, I have addressed it. I am now exposing a richer JS object populated with data from rd_kafka_error_t.

edenhill · 2021-04-06T19:36:07Z

src/producer.h

+  Baton InitTransactions(int32_t timeout_ms);
+  Baton BeginTransaction();
+  Baton CommitTransaction(int32_t timeout_ms);
+  Baton AbortTransaction(int32_t timeout_ms);


SendOffsetsToTransaction() is also required

I am not familiar with committing offsets, it is something I need to investigate. Do you think it is possible to merge this PR and leave that for future work?

* Expose the richer 'rd_kafka_error_t' object returned by Rdkafka to the JS code. * Add a test to cover the generated error object.

iradul · 2021-05-07T03:28:44Z

@YordanPavlov thanks for the PR! Also @edenhill thank you for reviewing this!

We'll need to do some more work for this to be merged.

Transactions are only valuable in consume-process-produce scenarios so as @edenhill mentioned SendOffsetsToTransaction is required and is a must.

I suggest you read through this Confluent page to get a better understanding.

Here are a few tasks that need to be addressed:

implement SendOffsetsToTransaction
e2e test must include a consumer with disabled auto commits and SendOffsetsToTransaction call
unless there is a reason not to, e2e tests should be merged into a single .spec.js file in e2e directory (no need for a new e2e_transactions directory and two specs)
ideally, all methods in javascript should be asynchronous
need to cover some docs on how to use this
need to add typescript definitions

…ent as argument but pointer is not transferred correctly to C++ land.

YordanPavlov · 2021-05-07T10:35:22Z

Thanks @iradul for considering this PR, I have done some work on trying to add the SendOffsetsToTransaction method, however I have some difficulties on how to transfer the consumer object from JS to C++ land. I have created a separate PR for only this feature:
santiment#2
I would appreciate it if you are someone else can have a look and provide guidance.

YordanPavlov · 2021-05-09T10:44:56Z

In the meantime I have addressed some of the other issues.

Added Typescript definitions.
Merged e2e transaction tests into e2e directory.

As for your comment 'ideally, all methods in javascript should be asynchronous' - as the C++ methods are synchronous I believe no callback is required in the JS code also.

iradul · 2021-05-10T02:13:50Z

Thanks.

I would appreciate it if you or someone else can have a look and provide guidance.

I'll work with you on this one. We'll integrate the whole feature within this PR, no need to make a new PR. You can go ahead and cherry-pick santiment#2 commits to here.

olegladyka · 2021-05-18T15:47:20Z

Hi,
I was working on implementation of "SendOffsetsToTransaction" from your branch, and managed to get it work.
You can see here the changes: https://github.com/olegladyka/node-rdkafka/compare/d1005e..7df42b1
However in my implementation "SendOffsetsToTransaction" receives additional parameter - array of TopicPartition, providing the ability to commit any offset, not just the current consumer offset.

Instead of fetching all offsets for all partitions for a consumer - expect to get them as argument on SendOffsetsToTransaction.

YordanPavlov · 2021-05-19T14:58:01Z

Hi,
I was working on implementation of "SendOffsetsToTransaction" from your branch, and managed to get it work.
You can see here the changes: https://github.com/olegladyka/node-rdkafka/compare/d1005e..7df42b1
However in my implementation "SendOffsetsToTransaction" receives additional parameter - array of TopicPartition, providing the ability to commit any offset, not just the current consumer offset.

Thank you for your contribution @olegladyka. I have merged my helper PR (the one specific for SendOffsetsToTransaction) and also copied your code, so now everything is in the current PR, as @iradul requested. Unfortunatelly even with your changes, I still get a core dump at this point:

./node_modules/nan/nan_object_wrap.h:32: static T* Nan::ObjectWrap::Unwrap(v8::Local<v8::Object>) [with T = NodeKafka::KafkaConsumer]: Assertion object->InternalFieldCount() > 0' failed.`

this happens on running the test for SendOffsetsToTransaction. Could you @olegladyka try it for yourself. I may have done some mistake you mentioned that it worked in your tests.

olegladyka · 2021-05-19T16:00:30Z

Hi,
I was working on implementation of "SendOffsetsToTransaction" from your branch, and managed to get it work.
You can see here the changes: https://github.com/olegladyka/node-rdkafka/compare/d1005e..7df42b1
However in my implementation "SendOffsetsToTransaction" receives additional parameter - array of TopicPartition, providing the ability to commit any offset, not just the current consumer offset.

Thank you for your contribution @olegladyka. I have merged my helper PR (the one specific for SendOffsetsToTransaction) and also copied your code, so now everything is in the current PR, as @iradul requested. Unfortunatelly even with your changes, I still get a core dump at this point:

./node_modules/nan/nan_object_wrap.h:32: static T* Nan::ObjectWrap::Unwrap(v8::Local<v8::Object>) [with T = NodeKafka::KafkaConsumer]: Assertion object->InternalFieldCount() > 0' failed.`

this happens on running the test for SendOffsetsToTransaction. Could you @olegladyka try it for yourself. I may have done some mistake you mentioned that it worked in your tests.

When you call producer.sendOffsetsToTransaction(consumer.position(), consumer, transactions_timeout_ms) in test, you need to use consumer.getClient() instead of consumer, like this:
producer.sendOffsetsToTransaction(consumer.position(), consumer.getClient(), transactions_timeout_ms)

iradul · 2021-05-23T03:15:29Z

Great work guys!
@YordanPavlov, I have a few fixes to add. To push those to this PR, you'll have to check Allow edits from maintainers.. Could you please do this? Thanks.

YordanPavlov · 2021-05-23T18:45:04Z

Thank you @olegladyka for pointing out the mistake on my end, indeed calling consumer.getClient() solved the problem. I did one more commit to tidy up the test.
@iradul I can't seem to find the Allow edits from maintainers checkbox, the PR is against upstream Master, could it be that you already have access.

iradul · 2021-05-24T02:46:27Z

Hey @YordanPavlov, the checkbox should be on the right-hand side on this same page under Notifications group.
Anyway, I just pushed a few commits on pr881 branch, and you can cherry-pick them, for example.

Before I merge this, we need to work a bit on e2e tests. Actually, I think, for now, you can remove send_offset_transaction.spec.js and fix producer-transaction.spec.js to work with my latest commits after you include them.

Also don't allow single offset

YordanPavlov · 2021-05-24T16:30:59Z

I cherry-picked your commits @iradul . Also added the small changes in the tests needed after the style changes. However I started getting some:

  1) Transactional Producer
       with consumer
         "after each" hook for "no message should be delivered if transaction is aborted":
     TypeError: Cannot read property 'call' of undefined
      at processImmediate (node:internal/timers:464:21)

errors on running the tests. I can't reproduce it running the tests one file by one but it happens when ran all together.

Some transaction end to end tests were failing for two reasons. * A delayed message from prevoius test was being read and braking up the logic of the test. Fixed by using a separate topic. * One more message was being produced than expected. Fixed.

YordanPavlov · 2021-06-16T12:19:43Z

@iradul I dug into those occasional test fails and fixed them. Please have a look at my most recent commit. Let's continue work on merging this PR.

iradul · 2021-06-25T21:12:08Z

We need to make those new APIs async since all methods can take a significant amount of time to complete.
I already have most of this code done, but since I can't push to your branch, I'm thinking of merging this as is and then add what's missing on top of that. Or you can keep cherry-picking commits from pr881.

Also, I'd like us to include a complete e2e transactional test, for instance, similar to this.

YordanPavlov · 2021-06-28T16:26:38Z

We need to make those new APIs async since all methods can take a significant amount of time to complete.
I already have most of this code done, but since I can't push to your branch, I'm thinking of merging this as is and then add what's missing on top of that. Or you can keep cherry-picking commits from pr881.

I have investigated why I am not seeing the option to grant you Edit access to the PR and the reason seems to be that the Fork I had originally done lives in an organization and not my personal namespace, so Github strips me of the ability to grant access. Any of the three options work for me though:

I can keep cherry-picking commits.
We can merge the commit and improve it further in another PR.
I can create a brand new PR out of my personal GitHub namespace where I would be able to grant you edit rights, but I am afraid that we would loose the history of the discussion here.

So just let me know how you would prefer to continue.

iradul · 2021-06-29T01:13:03Z

Great feedback. I like the third option. You can reference this PR in the opening comment, so all the history will be available and preserved.

YordanPavlov · 2021-07-13T15:00:26Z

Sorry for the delay guys. Here is the new PR:
#903
I used the opportunity to do some cleaning of the commits. Commits by maintainer are now allowed.

YordanPavlov added 3 commits April 1, 2021 13:43

Expose transaction methods from librdkafka.

53007a1

Reduce replication factor for transactions.

bcc26f4

Replication factor of transaction related options should be reduced to 1 so that we can test with a single broker.

New e2e tests added for transactions.

c3021a5

edenhill reviewed Apr 6, 2021

View reviewed changes

Expose Rdkafka transaction error to JS code.

d1005e5

* Expose the richer 'rd_kafka_error_t' object returned by Rdkafka to the JS code. * Add a test to cover the generated error object.

YordanPavlov mentioned this pull request Apr 20, 2021

Support transactions santiment/san-exporter#20

Merged

This was referenced Apr 22, 2021

Application doesn't finish gracefully after client.disconnect #5

Closed

#881 #884

Closed

'Send offsets to transaction' implemenation, still WIP. Producer is s…

783863c

…ent as argument but pointer is not transferred correctly to C++ land.

YordanPavlov added 2 commits May 9, 2021 13:09

Adding Typescript definitions.

ac3fa46

e2e_transactions tests merged into e2e directory

a07882d

YordanPavlov added 2 commits May 19, 2021 17:01

'send_offsets_transaction' implementation. Bug causes test to fail.

6c43abd

Offsets argument on SendOffsetsToTransaction

e46da02

Instead of fetching all offsets for all partitions for a consumer - expect to get them as argument on SendOffsetsToTransaction.

Fixes in transaction e2e tests.

4160443

Typescript definition extended with 'sendOffsetsToTransaction'

ac4ead3

iradul added 3 commits May 24, 2021 18:27

Stay consistent with librdkafka naming

02cd92c

Make groupMetadata accessible via friend class

9f272ad

Handle transaction errors but don't return

48f4160

iradul and others added 2 commits May 24, 2021 18:32

Expect javascript KafkaConsumer

6358fbe

Also don't allow single offset

Tests changed to comply with code. Timeout added on producing messages.

b4612ea

Fix occasionally failing tests.

b6eac9a

Some transaction end to end tests were failing for two reasons. * A delayed message from prevoius test was being read and braking up the logic of the test. Fixed by using a separate topic. * One more message was being produced than expected. Fixed.

YordanPavlov mentioned this pull request Jul 13, 2021

Support transactions #903

Merged

iradul closed this Jul 26, 2021

iradul pushed a commit that referenced this pull request Aug 10, 2021

Support transactions (#881 -> #903)

07660be

YordanPavlov mentioned this pull request Dec 19, 2023

Optimize block timestamp discovery. Integration tests. santiment/san-chain-exporter#173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support transactions #881

Support transactions #881

YordanPavlov commented Apr 1, 2021 •

edited

Loading

YordanPavlov commented Apr 1, 2021

edenhill Apr 6, 2021

YordanPavlov Apr 20, 2021

edenhill Apr 6, 2021

YordanPavlov Apr 20, 2021

iradul commented May 7, 2021 •

edited

Loading

YordanPavlov commented May 7, 2021

YordanPavlov commented May 9, 2021

iradul commented May 10, 2021

olegladyka commented May 18, 2021

YordanPavlov commented May 19, 2021

olegladyka commented May 19, 2021 •

edited

Loading

iradul commented May 23, 2021

YordanPavlov commented May 23, 2021

iradul commented May 24, 2021

YordanPavlov commented May 24, 2021

YordanPavlov commented Jun 16, 2021

iradul commented Jun 25, 2021

YordanPavlov commented Jun 28, 2021 •

edited

Loading

iradul commented Jun 29, 2021

YordanPavlov commented Jul 13, 2021

Support transactions #881

Support transactions #881

Conversation

YordanPavlov commented Apr 1, 2021 • edited Loading

YordanPavlov commented Apr 1, 2021

edenhill Apr 6, 2021

Choose a reason for hiding this comment

YordanPavlov Apr 20, 2021

Choose a reason for hiding this comment

edenhill Apr 6, 2021

Choose a reason for hiding this comment

YordanPavlov Apr 20, 2021

Choose a reason for hiding this comment

iradul commented May 7, 2021 • edited Loading

YordanPavlov commented May 7, 2021

YordanPavlov commented May 9, 2021

iradul commented May 10, 2021

olegladyka commented May 18, 2021

YordanPavlov commented May 19, 2021

olegladyka commented May 19, 2021 • edited Loading

iradul commented May 23, 2021

YordanPavlov commented May 23, 2021

iradul commented May 24, 2021

YordanPavlov commented May 24, 2021

YordanPavlov commented Jun 16, 2021

iradul commented Jun 25, 2021

YordanPavlov commented Jun 28, 2021 • edited Loading

iradul commented Jun 29, 2021

YordanPavlov commented Jul 13, 2021

YordanPavlov commented Apr 1, 2021 •

edited

Loading

iradul commented May 7, 2021 •

edited

Loading

olegladyka commented May 19, 2021 •

edited

Loading

YordanPavlov commented Jun 28, 2021 •

edited

Loading